Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrected str_trunc() to pass correct 'left' and 'center' values to str_sub() when internal variable width... is 0 or 1 #513

Closed
wants to merge 2 commits into from

Conversation

eauleaf
Copy link

@eauleaf eauleaf commented Jun 7, 2023

Correction should close Issue #512 and #494.

Note: in my tests, the #494 pull request also fixes the issue. (However, the #494 fix is ~10% slower, likely due to the internal function definition, in the standard case where no 'left' or 'center' replacements of zero length occur. #494 is ~5% faster in the edge case corrected here, likely due to the internal function returning an empty sting directly instead of snipping to an empty string.)

Below is the summary of the issue:

  • when nchar(width) is 1 more than nchar(ellipsis), side 'center' fails to return correct result
  • when nchar(width) is == to nchar(ellipsis), sides 'center' and 'left' fail to return correct result

Both cases happen because:
str_sub(string[too_long], -width..., -1))
becomes:
str_sub('characters', 0, -1))
which returns all 'characters' rather than the correct number

Example:
c('', 'a', 'aa', 'aaa', 'aaaa', 'aaaaaaa') |> str_trunc(3, 'center', "..")

Incorrectly returns:
[1] "" "a" "aa" "aaa" "a..aaaa" "a..aaaaaaa"

Rather than:
[1] "" "a" "aa" "aaa" "a.." "a.."

@eauleaf eauleaf changed the title Corrected str_trunc() to pass str_sub() correct 'left' and 'center' parameters when internal variable width... is 0 or 1 Corrected str_trunc() to pass correct 'left' and 'center' values to str_sub() when internal variable width... is 0 or 1 Jun 7, 2023
added a couple more tests for completeness
@hadley
Copy link
Member

hadley commented Aug 4, 2023

Duplicate of #515

@hadley hadley marked this as a duplicate of #515 Aug 4, 2023
@hadley hadley closed this Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants